Wow the improvement is very big. Could you provide script you used to benchmark rocm vs modnn + script benchmark rocm vs miopen to here or comment on your Jira ticket (recommend this method because recently our workflow required uploading benchmark script to comment of Jira ticket). Did you find any specific reasons that the improvement in some cases is very big? For example I saw linear backward is around 200x improvement compared to ROCm

Wow the improvement is very big. Could you provide script you used to benchmark rocm vs modnn + script benchmark rocm vs miopen to here or comment on your Jira ticket (recommend this method because recently our workflow required uploading benchmark script to comment of Jira ticket). Did you find any specific reasons that the improvement in some cases is very big? For example I saw linear backward is around 200x improvement compared to ROCm

Very nice of you to remind me. I added the script to JIRA ticket. There are no special reasons; my MIOpen's performance is similar to Modnn's performance, and I made some constraints to eliminate low-performance cases.

Wow the numbers look very impressive. But Some of that don't make sense to me, for example 99.29x in fp16 and 2.59x in fp32 (Bilinear bwd). Can you figure out why this much difference happen?

It looks like one case with 1800x improvement ruined the geomean but please check.

I read benchmark number in 5d backward nearest and it looks like the last (very) small tests contributed too much to your average improvement. Overall big tests produced slower results (around 0.8 - 1x improvement) but the last very small tests with only 2048 and 4096 threads have 10-14x improvement, and then average improvement for this case is 2.66x, which I think is not fair. Could you create more tests for that cases with total threads between 100k and 5M to see if it is faster or not?

Run make analyze and fix the warnings existed in your file in this PR.

Please don't update this branch to newest commit in upstream repo. Because in this PR you choose to merge to develop-moreh branch, now it will show '752 file changed' and I cannot review your code =))) (I clicked Files changed button and my Edge crashed after that 😡) You can consider creating a new PR which merge to moreh-dev:develop if you want

Please don't update this branch to newest commit in upstream repo. Because in this PR you choose to merge to develop-moreh branch, now it will show '752 file changed' and I cannot review your code =))) (I clicked Files changed button and my Edge crashed after that 😡) You can consider creating a new PR which merge to moreh-dev:develop if you want

If I don't merge to develop, I can't fix issues which you mentioned above about MIOPEN_INTERNALS_EXPORT or Gtest new convention. You reap what you sow, my dear. Love your thoughtful reviews btw <3 !

Please don't update this branch to newest commit in upstream repo. Because in this PR you choose to merge to develop-moreh branch, now it will show '752 file changed' and I cannot review your code =))) (I clicked Files changed button and my Edge crashed after that 😡) You can consider creating a new PR which merge to moreh-dev:develop if you want

If I don't merge to develop, I can't fix issues which you mentioned above about MIOPEN_INTERNALS_EXPORT or Gtest new convention. You reap what you sow, my dear. Love your thoughtful reviews btw <3 !

Ok bro 🥲 if that is you can ignore my comments which can only fix when updating to new branch, i thought that macro existed from older branch, my bad 🥹 please revert back to previous commit before you updated to latest commit on upstream repo because now I have no ways to review your PR 😂😂 And i rethink it’s not a good idea to open a new PR to moreh-dev:develop because that branch is not synced up to latest commit on upstream repo, if your last commit you update to branch develop on moreh-dev then it is ok to open a new PR, but if you update to branch develop on rocm then it will show up many new files not related to your operation when opening new PR to moreh-dev:develop

Overall it looks good now. Make sure to check this comment #41 (comment) and update 5D backward nearest benchmark result after you create more test cases. Also I found that in the newest commit d15fbc5 you commented out IsOverRocmNearestBwd and IsOverRocmNearestFwd, is this accidental or on purpose?

IsOverRocmNearestBwd

I commented for re-run benchmark, updated 5D backward, check it out

LGTM! Thank you for your hard work in this PR, and make sure to ask for review from other reviewers.

@kyeonghwanryu @DuongQLee Do you want to add more reviews? If not, can I close this PR?

Sorry it seems I missed the comment. I'm closing it.